Conversation
…for support Co-authored-by: cdfmlr <45259230+cdfmlr@users.noreply.github.com>
… support Co-authored-by: cdfmlr <45259230+cdfmlr@users.noreply.github.com>
Co-authored-by: cdfmlr <45259230+cdfmlr@users.noreply.github.com>
Co-authored-by: cdfmlr <45259230+cdfmlr@users.noreply.github.com>
|
@copilot Python 3.6 & 3.7 supports are claimed but had never been tested. Cover them with the test matrix or you should at least document the untested fact in README. And again review the changes and all the code. Make sure everything works correctly, and no unexpected behaviors drifting introduced (except new features and bug fixes). Improve the PR description, documents all the changes in necessary details. |
…DME updated Co-authored-by: cdfmlr <45259230+cdfmlr@users.noreply.github.com>
Done in commit Python 3.6 & 3.7 coverage:
Code review — no unexpected behaviour changes introduced. All existing tests pass (21/21), 0 CodeQL alerts. Every change in the PR is either a genuine bug fix (e.g. |
cdfmlr
left a comment
There was a problem hiding this comment.
... tired, will see these latrr
|
|
||
| if progress.head is not None: | ||
| process = parse(self.ast_object.body, **kwargs) | ||
| if process.head is not None: |
There was a problem hiding this comment.
@copilot why it used to parsed twice? was it by design or just a dirty implementation? double check may our fix here hurt others on somewhere else? and this modification is not described in the PR description. You should document literally all the changes.
There was a problem hiding this comment.
It was a dirty implementation bug, not by design. The original code:
progress = parse(self.ast_object.body, **kwargs) # result used only for the guard
if progress.head is not None:
process = parse(self.ast_object.body, **kwargs) # identical call, result actually usedprogress was used only for the head is not None guard and then thrown away. The second call stored in process was what actually wired the graph. There is no design reason to parse twice — parse() is deterministic for a given input (same body, same kwargs → same structure). The only side-effect of calling twice was inflating node IDs via itertools.count. The fix (commit 76e46c6) removes the redundant first call and uses the single result directly. No other callsite is affected.
Also documented this in the PR description.
| InputOutputNode.__init__(self, InputOutputNode.OUTPUT, self.ast_to_source().lstrip("return")) | ||
| # ast_to_source() gives "return <value>" or bare "return". | ||
| # Strip the "return" keyword: use prefix removal to avoid lstrip()'s | ||
| # character-set semantics (lstrip("return") strips individual chars, not the word). | ||
| source = self.ast_to_source() | ||
| if source.startswith("return"): | ||
| source = source[len("return"):].lstrip() | ||
| InputOutputNode.__init__(self, InputOutputNode.OUTPUT, source) |
pyflowchart/ast_node.py
Outdated
| # self.head = TransparentNode(self) | ||
| # fuck the multi inheritance,,, my brain is buffer overflowing | ||
| # god bless the jetbrains helped me figure out this overstep | ||
| # well, never mind. I believe that NodesGroup.__init__() | ||
| # is the right way to set it up as well as self.head properly. | ||
| # NodesGroup.__init__() is the correct way to initialise the head here. | ||
| # The transparent_head acts as a placeholder until cases are parsed. |
There was a problem hiding this comment.
Restored in 76e46c6. The commented-out # self.head = TransparentNode(self) and the explanation of why it was abandoned (MRO issues with multi-inheritance) are back — only the profane word is removed.
There was a problem hiding this comment.
oh, sorry for the misdirection,. i mean, the previous profance comments is not cool, your fix is the right one. @copilot
pyflowchart/test.py
Outdated
| for name in internal: | ||
| self.assertNotIn(name, pyflowchart.__all__, msg=f"internal '{name}' leaked into __all__") | ||
|
|
||
| def test_match_simplify_kwarg_forwarded_into_case_bodies(self): |
There was a problem hiding this comment.
but why ai tends to write test_names_in_such_a_giant_length
There was a problem hiding this comment.
Renamed to test_match_kwargs_forwarded in 76e46c6.
|
|
||
| if progress.head is not None: | ||
| process = parse(self.ast_object.body, **kwargs) | ||
| if process.head is not None: |
There was a problem hiding this comment.
@copilot why it used to parsed twice? was it by design or just a dirty implementation? double check may our fix here hurt others on somewhere else? and this modification is not described in the PR description. You should document literally all the changes.
| InputOutputNode.__init__(self, InputOutputNode.OUTPUT, self.ast_to_source().lstrip("return")) | ||
| # ast_to_source() gives "return <value>" or bare "return". | ||
| # Strip the "return" keyword: use prefix removal to avoid lstrip()'s | ||
| # character-set semantics (lstrip("return") strips individual chars, not the word). | ||
| source = self.ast_to_source() | ||
| if source.startswith("return"): | ||
| source = source[len("return"):].lstrip() | ||
| InputOutputNode.__init__(self, InputOutputNode.OUTPUT, source) |
Co-authored-by: cdfmlr <45259230+cdfmlr@users.noreply.github.com>
pyflowchart/test.py
Outdated
| # Tests for bug fixes # | ||
| # ------------------------------------------------------------------ # | ||
|
|
||
| def test_from_code_invalid_field_raises_value_error(self): |
There was a problem hiding this comment.
Shortened in a98d6c8:
test_from_code_invalid_field_raises_value_error→test_invalid_field_raisestest_from_code_empty_raises_value_error→test_empty_code_raisestest_find_field_invalid_path_returns_empty_body→test_find_field_invalidtest_detect_decode_low_confidence_falls_back_to_utf8→test_detect_decode_low_confidence
Co-authored-by: cdfmlr <45259230+cdfmlr@users.noreply.github.com>
|
@copilot all modifications are reviewed and lgtm and all the checks passed. please finally bumping the version to 0.5.0 alpha, ur, no, beta 1 directly |
Co-authored-by: cdfmlr <45259230+cdfmlr@users.noreply.github.com>
Done in |
pyflowchart claimed Python 3.6+ support but had never been tested against it, used a
setup.py-only packaging setup with nopyproject.toml, carried several silent runtime bugs, and had no CI test matrix at all. This PR addresses all of it.Version
Bumped to 0.5.0b1 (
setup.pyandpyproject.toml).Packaging
pyproject.toml(PEP 517/621): build system, classifiers for Python 3.7–3.14,[dev]extra,pyflowchartconsole-script entry point, pytest config, Bug Tracker URLsetup.py: drop Python 3.6 classifier (not supported bysetuptools>=61), add Python 3.7–3.14 classifiers, updatepython_requiresto>=3.7, addentry_pointsfor the console script.gitignore: removepyproject.tomlexclusion (a leftover "poetry experiment" comment was silently preventing it from ever being committed)README.md: updated to state "requires Python 3.7+ and is CI-tested on Python 3.7 through 3.14" — Python 3.6 is not supported becausesetuptools>=61.0(required by the PEP 621[project]table) itself requires Python ≥ 3.7Bug fixes
ast_node.pyimport typing— unused bare import removed;Listadded explicitly tofrom typing import …parse()call inLoop.parse_loop_body()— original code calledparse(self.ast_object.body, **kwargs)twice: the first result (progress) was used only for thehead is not Noneguard then discarded, and a second identical call (process) was used for the actual graph wiring. This was not intentional design — a dirty implementation that inflated node IDs viaitertools.count. Fixed by removing the redundant first call and using a single result throughout. The fix is safe:parse()is deterministic for a given input and the single result is used consistently.lstrip("return")inReturnOutput— strips individual characters{r,e,t,u,n}, not the keyword; fixed tosource[len("return"):].lstrip()_ast.Raisenot handled — fell through toCommonOperationand connected forward;raiseis terminal likebreak; added to__ctrl_stmtsmapped toBreakContinueSubroutine_ast.YieldFromnot handled — fell through toCommonOperationinstead of an I/O output node; added to__ctrl_stmtsmapped toYieldOutputAsyncFunctionDef/AsyncFornot handled — fell through toCommonOperation; added to__func_stmts/__loop_stmtsrespectively, mapped to the sameFunctionDef/Loopclasses; assert guards inFunctionDefArgsInput.func_args_str()andFunctionDef.parse_func_body()updated to include_ast.AsyncFunctionDefexcept IndexError or AttributeErrorinMatch.__init__— Python evaluates this asexcept IndexError;AttributeErrorwas never caught; fixed toexcept (IndexError, AttributeError)MatchCase.parse_body()— used bare_ast.match_caseinstead of_ast_match_case_talias; calledparse(body)without**kwargs, silently droppingsimplify/conds_aligninside every match-case bodyMatch.parse_cases()— bare_ast.Matchinstead of_ast_Match_taliasMatch.__init__()comments — restored the commented-out# self.head = TransparentNode(self)alternative with a note explaining whyNodesGroup.__init__()is used instead (direct assignment hits multi-inheritance MRO issues); removed only the profane wordparse()—type(x) == Yreplaced withisinstance(x, Y)for both Match and Expr checksflowchart.pyassertstatements infrom_code()— silently skipped bypython -O, producing a crypticAttributeError; replaced withraise ValueErrorassertinfind_field_from_ast()— used insidetry/except (AttributeError, AssertionError)to detect a missing field; disabled in-Omode, causing the wrong AST node to be silently returned and garbage flowcharts to be produced; replaced withif/raise ValueErrorand tightenedexceptclause__main__.pyconfidence < 0.9indetect_decode()— raisesTypeErrorwhen chardet returnsconfidence=Noneon unrecognised byte streams; fixed toif not confidence or confidence < 0.9:__init__.py__all__—from pyflowchart import *leakedtime,uuid,itertools,List,TypeVar,debug, and every private symbol; 26 public names now declared explicitlyCI / GitHub Actions
codeql-analysis.yml:checkout@v2→@v4,codeql-action/*@v1→@v3, addpermissionsblockTestPyPi-publish.yml/release-publish.yml:checkout@master(unpinned) →@v4,setup-python@v1→@v5,pypi-publish@master(unpinned) →@release/v1, Python 3.7 (EOL) → 3.12, addpermissionswithid-token: writetests.yml: CI test matrix covering the full supported range — Python 3.7 onubuntu-22.04(the only runner with Python 3.7 in its hosted tool cache) and Python 3.8–3.14 onubuntu-latest, running on every push/PRTests
7 new tests added covering every fix with no prior test:
test_invalid_field_raisesValueError(notAssertionError) for bad fieldtest_empty_code_raisesValueErrorfor empty codetest_find_field_invalid-Omode)test_detect_decode_utf8test_detect_decode_low_confidenceNone/zero confidenceTypeErrorfixtest_public_api_all_complete__all__completeness + no internal leakagetest_match_kwargs_forwardedsimplifypropagated into match-case bodies💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.